Add the OpenStack load balancer deployment options by tomassedovic · Pull Request #7209 · openshift/openshift-ansible

tomassedovic · 2018-02-20T09:28:23Z

This adds a support for automatically creating and configuring an LBaas
v2 (Octavia) as well as the VM-based fallback.

Health checks and additional protocol support will be added in follow-up
patches.

bogdando · 2018-02-20T09:38:14Z

playbooks/openstack/advanced-configuration.md

 prerequisite packages to be installed before to deploy an OpenShift cluster.
 Those are ignored though, if the `manage_packages: False`.

-## Multi-master configuration


Doesn't the multi-master configuration case require anything to be noted in the docs any more?

I don't think so. Do you have anything concrete in mind?

I meant, anything specific to the multi-master. IIUC, The cloud-native LBaaS has nothing to multi-master, right? Openshift-ansible configures it out of box instead.

I don't think so. Do you have anything concrete in mind?

Expanded in the comments below, wrt the default solution for openshift-ansible. Seems like not supported from now on, right?

bogdando · 2018-02-20T09:42:00Z

playbooks/openstack/advanced-configuration.md

+
+    openshift_openstack_use_vm_load_balancer: true
+
+In either case, you can get the IP addresses for the API and routers by


nit: is it in fact the IP of LB (frontend) for those? Let's please clarify,

Yeah, it's the IP of the public LB. I've updated the docs to make this clearer.

bogdando · 2018-02-20T09:43:29Z

roles/openshift_openstack/tasks/populate-dns.yml

    - hostvars[groups.masters[0]].openshift_master_cluster_public_hostname is defined
    - openshift_openstack_num_masters > 1

+# TODO(shadower): add the API and apps IP addresses from the Heat stack here.


why we don't want to implement that in the patch scope? Should we note something for this in the docs?

Because we need to read the Heat output in the dynamic inventory.

That's being added in this PR:

#6319

If that gets merged first, I'll add it here.

Alright, that PR got merged so my next revision will set the DNS records.

bogdando · 2018-02-20T09:46:38Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

+{% if openshift_openstack_use_lbaas_load_balancer %}
+    value: { get_attr: [lb_floating_ip, floating_ip_address] }
+{% elif openshift_openstack_use_vm_load_balancer %}
+    # NOTE(shadower): The VM-based loadbalancer only supports master nodes


this prolly worth of a note in the docs

Good point, done

bogdando · 2018-02-20T09:47:58Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

+    description: IP address of the apps/router endpoint
+{% if openshift_openstack_use_lbaas_load_balancer %}
+    value: { get_attr: [lb_floating_ip, floating_ip_address] }
+{% elif openshift_openstack_use_vm_load_balancer %}


we could as well just put a single else clause

+1 to this; I think it's easier to read if these two if/elif/else blocks are combined into one

bogdando · 2018-02-20T09:50:08Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

      policies: {{ openshift_openstack_infra_server_group_policies }}
 {% endif %}
-{% if openshift_openstack_num_masters|int > 1 %}
+{% if openshift_openstack_use_vm_load_balancer %}


This changes the default behavior for multiple masters. With openshift_openstack_use_vm_load_balancer: false by default, there would be no more loadbalancer provisioned for openshift-ansible multi-master out-of-box solution. So we in fact ditched it. Is that what we want?

Yes. You should opt-into a load balancer and when doing so, choose which one to use.

This also lets you use an external LB without wasting resources.

Out of curiosity, what will happen if multiple masters are specified without a load balancer? Is that something we want to check against?

The cluster would work just fine. You could talk to either of the master nodes to access the API or UI and similarly to either of the infra nodes for the public routes.

I've added an explanation to the docs.

bogdando

Do we really have to ditch the default openshift-ansible multi-master solution for the preference of the native LBaaS? IIUC, that's what this implements for that case.

tomassedovic · 2018-02-20T09:56:00Z

We now have two options, each with it's advantages and drawbacks (the multimaster solution doesn't handle openshift routes which is kind of a big deal) so we're removing any default and letting you opt into what you want.

This is what you'd have to do with the manual installation as well: you don't get the VM-based solution "for free".

bogdando · 2018-02-20T09:56:28Z

roles/openshift_openstack/templates/heat_stack_server.yaml.j2

+          - server
+          - addresses
+          - { get_param: net_name }
+          - 0


nit: shall we place a FIXME for multiple private networks support?

bogdando · 2018-02-20T10:00:58Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

+
+
+{% if openshift_openstack_use_lbaas_load_balancer %}
+  # NOTE: we're using the same LB for both API and Routers


ditto, let's note for the docs as well

Good point, done.

bogdando · 2018-02-20T10:01:42Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

+  api_lb_pool:
+    type: OS::Neutron::LBaaS::Pool
+    properties:
+      lb_algorithm: ROUND_ROBIN


nit: we prolly want this configurable

The previous default (the VM-based LB) was not configurable either, so I'm tempted to leave this as is for now. But I've added a TODO to make it configurable in the future.

bogdando · 2018-02-20T10:02:46Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

+  router_lb_pool:
+    type: OS::Neutron::LBaaS::Pool
+    properties:
+      lb_algorithm: ROUND_ROBIN


bogdando

This is really well done! I have mostly minor concerns for docs changes.

bogdando · 2018-02-20T10:05:07Z

@tomassedovic ack

the multimaster solution doesn't handle openshift routes which is kind of a big deal

That works for me, thanks. Let's please just explain that and/or how this impacts the opinionated choices we made and defaults we set, in the docs!

tzumainn

Hi! I've tested this with lbaas, and things look good! I've left a few pretty nit-picky comments that aren't all that high priority.

tzumainn · 2018-03-02T14:24:17Z

playbooks/openstack/advanced-configuration.md

+
+If you run multiple master or infra nodes, you want to put a load balancer in
+front of them.  We can configure an Octavia or Neutron LBaaS V2 load balancer
+automaticaly.


minor nit: "automatically"

tzumainn · 2018-03-02T14:26:11Z

playbooks/openstack/advanced-configuration.md

+
+All you need to do is put this in your `inventory/group_vars/all.yml`:
+
+    openshift_openstack_use_lbaas_load_balancer: true


I was about to leave a separate comment asking if we want to put this option in the sample inventory file; then I remember you saying that you'd like to start emptying that out and simply have the possible parameters listed in the documentation, correct?

If so, I think we may want to organize that documentation a bit differently, so it's easier for someone to simply skim the list of possible parameters to find the ones that they care about. I can take a shot at that if you'd like.

Yeah, I'd like to prune some of the less important and space-consuming options out.

But this one is actually quite important, I think. I'll add it and we can clean it all up in another patch.

tzumainn · 2018-03-02T14:27:38Z

roles/openshift_openstack/tasks/check-prerequisites.yml

+  fail:
+    msg: >
+      Only one of `openshift_openstack_use_lbaas_load_balancer` and
+      `openshift_openstack_use_vm_load_balancer` can be set at a time.


nit: technically they can both be set; they just can't both be true, correct?

Good point, fixed.

tzumainn · 2018-03-02T14:28:26Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

+    description: IP address of the apps/router endpoint
+{% if openshift_openstack_use_lbaas_load_balancer %}
+    value: { get_attr: [lb_floating_ip, floating_ip_address] }
+{% elif openshift_openstack_use_vm_load_balancer %}


+1 to this; I think it's easier to read if these two if/elif/else blocks are combined into one

tzumainn · 2018-03-02T14:29:39Z

roles/openshift_openstack/templates/heat_stack.yaml.j2

      policies: {{ openshift_openstack_infra_server_group_policies }}
 {% endif %}
-{% if openshift_openstack_num_masters|int > 1 %}
+{% if openshift_openstack_use_vm_load_balancer %}


Out of curiosity, what will happen if multiple masters are specified without a load balancer? Is that something we want to check against?

tomassedovic · 2018-03-21T11:42:13Z

The latest update rebases (and resolves the conflicts) and it should address all the comments that were raised.

I have a few questions:

Since I'd like to treat Heat mostly as an implementation detail, how about we document how to query the public IP addresses of the load balancer using the dynamic inventory instead of openstack stack output show? We can even print them out at the end of the install playbook. Thought?
Mostly a question to @celebdor can we merge the Kuryr LB with this new one? AFAICS there shouldn't be anything preventing us to use a single loadbalancer for everything, but I'd like to hear your thoughts.

celebdor · 2018-03-21T11:47:22Z

We can print it from the inventory at the end of the install playbook
It should definitely be merged with the kuryr one.

tomassedovic · 2018-03-26T15:19:57Z

Ok, this rebase should address both comments. The API LB is now merged with the Kuryr one, we print out the endpoint IP addresses at the end of the installation and the router LB is now separate and handles HTTP as well as HTTPS.

This adds a support for automatically creating and configuring an LBaas v2 (Octavia) as well as the VM-based fallback. We reuse the API LB required by Kuryr and we add a separate LB for the router. Health checks and additional protocol support will be added in follow-up patches.

tzumainn · 2018-04-06T16:27:43Z

Tested again, things seem to work \o/

tzumainn · 2018-04-06T16:27:46Z

/lgtm

openshift-ci-robot · 2018-04-06T18:00:30Z

@tomassedovic: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_crio	`034a08b`	link	`/test crio`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

tomassedovic · 2018-04-09T05:54:43Z

/retest

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 20, 2018

bogdando reviewed Feb 20, 2018

View reviewed changes

bogdando suggested changes Feb 20, 2018

View reviewed changes

bogdando reviewed Feb 20, 2018

View reviewed changes

bogdando suggested changes Feb 20, 2018

View reviewed changes

tzumainn reviewed Mar 2, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2018

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 24, 2018

openshift-ci-robot assigned tzumainn Apr 6, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2018

openshift-merge-robot merged commit c2c5f02 into openshift:master Apr 9, 2018

tomassedovic deleted the lbaas branch April 9, 2018 09:22


		openshift_openstack_use_vm_load_balancer: true

		In either case, you can get the IP addresses for the API and routers by



		{% if openshift_openstack_use_lbaas_load_balancer %}
		# NOTE: we're using the same LB for both API and Routers


		All you need to do is put this in your `inventory/group_vars/all.yml`:

		openshift_openstack_use_lbaas_load_balancer: true

Comments

Conversation

tomassedovic commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdando Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdando Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdando Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdando left a comment

Choose a reason for hiding this comment

Uh oh!

tomassedovic commented Feb 20, 2018

Uh oh!

bogdando Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdando left a comment

Choose a reason for hiding this comment

Uh oh!

bogdando commented Feb 20, 2018

Uh oh!

tzumainn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bogdando Feb 20, 2018 •

edited

Loading

bogdando Feb 20, 2018 •

edited

Loading

bogdando Feb 20, 2018 •

edited

Loading

bogdando Feb 20, 2018 •

edited

Loading

openshift-ci-robot commented Apr 6, 2018 •

edited

Loading